Add HTTP retry handling into task SDK api.client #45121
Conversation
There was a problem hiding this comment.
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (1)
task_sdk/tests/api/test_client.py:101
- There is an extra single quote in the mounts dictionary key. It should be mounts={'http://': httpx.MockTransport(mock_handle_request)}.
mounts={'http://': httpx.MockTransport(mock_handle_request)},
jscheffl
force-pushed
the
feature/44355-add-retry-handling-into-task-sdk-api-client
branch
from
bc58ae6 to
5399ace
Compare
| API_RETRIES = int(os.getenv("AIRFLOW__WORKERS__API_RETRIES", 10)) | ||
| API_RETRY_WAIT_MIN = int(os.getenv("AIRFLOW__WORKERS__API_RETRY_WAIT_MIN", 1)) | ||
| API_RETRY_WAIT_MAX = int(os.getenv("AIRFLOW__WORKERS__API_RETRY_WAIT_MAX", 90)) | ||
|
|
There was a problem hiding this comment.
Two things here:
- Currently, the API_RETRIES, API_RETRY_WAIT_MIN, and API_RETRY_WAIT_MAX are directly cast to integers. This can raise a ValueError if the environment variables are not set correctly. Can we add validation or some fallback defaults (In case of error, if we don't want to fail)?
- Let's also have a check,
API_RETRY_WAIT_MIN < API_RETRY_WAIT_MAXand raise alarm if not ?
There was a problem hiding this comment.
Good points. But as in the comments above it is referring to a "proper config mechanism" which is missing. Would leave it up there.
The values at the end are passed to tenacity, even there in the logic it seems to be no validation. If you "mess-up" the values then it will use the max as minimum it seems. Not too bad.
For me it is okay like this, except if @kaxil or @ashb would also enforce me to make it bullet-proof here. My aim was that it is working by default but that the defaults can be over-ridden. No "official" config but something that can be tweaked if you are looking into code.
jscheffl
force-pushed
the
feature/44355-add-retry-handling-into-task-sdk-api-client
branch
from
5399ace to
23614f0
Compare
closes: #44355
Add HTTP retry handling to Task SDK.